Deferred: Deprecate deferred.pipe() method #93
Conversation
3dc6bd5 to
7691f7f
Compare
| jQuery.Deferred = function() { | ||
| var deferred = oldDeferred.apply( this, arguments ); | ||
|
|
||
| deferred.pipe = function() { |
There was a problem hiding this comment.
So this will add the pipe method to the deferred, but not to the promise - using .then or .promise would yield a promise that does not have a pipe method unless I'm mistaken.
In order to get the method available everywhere it needs to be, this probably needs to look like
function pipe() { ... }
deferred.pipe = deferred.promise().pipe = pipe;There was a problem hiding this comment.
Oh yeah, I totally missed that case! I need to go back and double check some of the edge cases since for example a Deferred can have a different object applied as its prototypical promise object and none of that is being covered here.
|
I still feel like I'm missing some cases here, although it doesn't need to be bulletproof. @gibson042 what do you think? |
| jQuery.Deferred = function() { | ||
| var deferred = oldDeferred.apply( this, arguments ); | ||
|
|
||
| // Don't add this method if the current jQuery doesn't provide it |
There was a problem hiding this comment.
I think you mean "only add this method if the current jQuery doesn't provide it"
Edit: Wait, I think I'm confused....shouldn't migrate be adding it always for back-compat in addition to overriding it for the warn?
There was a problem hiding this comment.
Overriding it for the warning. I didn't want to add .pipe if, for example, they were using jQuery 1.5 because .pipe wasn't there and maybe they're doing a feature detect or something. I suppose I could always add it but it seemed better to NOT shim it if the original Deferred didn't have it.
But talking to the rubber ducky now I realize that I'm not correctly covering the version 1.6 to 1.8 region where we had .then but it didn't return a new promise. In those cases we should use the existing .pipe which does return a new promise.
There was a problem hiding this comment.
I think I was confusing myself; I was making the false assumption that .pipe was being removed in 3.0, but I realize that isn't the case. What you said about older versions is def true though
|
You're probably safer with something like this: var Deferred = jQuery.Deferred;
jQuery.Deferred = function( func ) {
var deferred = Deferred();
var pipe = deferred.pipe;
if ( pipe ) {
deferred.pipe = deferred.promise().pipe = function() {
migrateWarn( "deferred.pipe() is deprecated" );
return pipe.apply( this, arguments );
};
}
if ( func ) {
func.call( deferred, deferred );
}
return deferred;
}; |
|
Thanks @jaubourg! It makes sense to wait until we've shimmed the |
|
I like @jaubourg's code and would take it even farther, dropping the By the way, should I make an issue/PR for that? |
|
I should be able to get to it in the next day or two. Thoughts on the unit tests? |
Hmmm, you'll need to be async because of |
|
Seems like now would be a good time to upgrade QUnit as well then. Let the yak shaving begin! |
|
Closed in favor of #101 |
It still hasn't been closed. :) I'm closing it then. |
Ref #89